Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: (do not merge yet) Rewrite readme #76

Closed
wants to merge 2 commits into from
Closed

WIP: (do not merge yet) Rewrite readme #76

wants to merge 2 commits into from

Conversation

GhostLyrics
Copy link

Pull Request Checklist

Is this in reference to an existing issue? Yes, #51

General

  • Update Changelog following the conventions laid out on Keep A Changelog

  • Update README with any necessary configuration snippets

Purpose

  • Convert Readme to new style
  • Flesh out details (e.g. deps)

Known Compatablity Issues

None.

@GhostLyrics
Copy link
Author

@majormoses I've made some notes while documenting the first check. Let me know what you think about the style and please try to reply to the notes.

  • please default to TLS on in 2017
  • some values are labelled with TODO, because I didn't know better
  • parameters have been sorted alphabetically but I put ones starting with 2 dashes at the bottom that didn't have short versions, otherwise they'll always float to the very top of the list
  • let me know if you think this style is ok for boolean-style parameter or it should be -e VALUE instead

Generally I try to follow this guide I wrote some time ago when rewriting sensu READMEs, but there is no precedent for boolean-style parameters, which is why I'm asking for input.
https://github.com/GhostLyrics/sensu-plugin-doc-draft

@majormoses
Copy link
Member

@GhostLyrics first off thank you, there is really a lack of people in the community willing to do this kind of improvement and there are not enough maintainers to do this for everyone. Here is my stance: we already document each parameter in the checks themselves and I want to avoid the duplication required to stick them into readme. I think what I think is helpful is to have a few basic examples for checks that people seem to have trouble with but what every option does should really be maintained in the check and can be retrieved with --help or opening up the code. and reading what the options are. It's possible we might need to spend time enhancing the descriptions for each argument which I think is a better use of time as it benefits anyone willing to help themselves.

Regarding things like:

please default to TLS on in 2017

I wholeheartedly agree though I would prefer we change the default in the check to be secure by default and have users consciously change this. Obviously would require a major release as that would be a breaking change.

-w true|false, --whole-response true|false(default:false`)

This seems reasonable though I think I would prefer writing it like this though I certainly don't feel super strong about this:
-w (true|false), --whole-response (true|false) (default: false)

If we wish to do document all this in the readme I might suggest a more tabular approach like this:

<table>
  <tr>
    <th> Option </th>
    <th> Description </th>
    <th> Short </th>
    <th> Long </th>
    <th> Default </th>
    <th> Type </th>
    <th> Extra Notes </th>
  </tr>

  <tr>
    <td> insecure </td>
    <td> Disables certificate validation (sigh there was no description...) </td>
    <td> `-k` </td>
    <td> `--insecure`
    <td> false </td>
    <td> Boolean </td>
    <td>
      This is the problem here is that there is no description. While some options this is very obvious its intention others are less so without looking at the code (I don't mean looking at the options in the code but what it actually does). I'd argue we should change sensu to reject plugin options that do not have a description. This should include some gotchas, examples, or be nil
    </td>
  </tr>
</table>

I have not spoken to other maintainers about this so this is my personal opinion and not the collective guidance. Asking for it now, @sensu-plugins/commit-bit please weigh in here so we can get a broader perspective.

@GhostLyrics
Copy link
Author

GhostLyrics commented Jun 7, 2017

we already document each parameter in the checks themselves and I want to avoid the duplication required to stick them into readme. I think what I think is helpful is to have a few basic examples for checks that people seem to have trouble with but what every option does should really be maintained in the check and can be retrieved with --help or opening up the code. and reading what the options are. It's possible we might need to spend time enhancing the descriptions for each argument which I think is a better use of time as it benefits anyone willing to help themselves.

Two points here:

  • many if not most scripts do not show any description for parameters when running with --help. This can be fixed via many, many PRs in many projects. Not a problem - I can start doing that too.
  • I personally, in my highly subjective opinion, think that it's simply not great documentation if I either have to dive into the code OR install the tool to check if a certain functionality is present, hence my drive to add them to the readme - as I've seen and enjoyed in my role model in terms of documentation (https://github.com/voxpupuli/puppet-unattended_upgrades)

@majormoses
Copy link
Member

majormoses commented Jun 7, 2017

many if not most scripts do not show any description for parameters when running with --help. This can be fixed via many, many PRs in many projects. Not a problem - I can start doing that too.

This makes me sad, if you ask me this is a prerequisite to good documentation. This is probably the first thing we should fix on our road to not sucking at documentation. Also this might reduce the number of requests we get while we determine how we want to do "external" documentation.

simply not great documentation if I either have to dive into the code OR install the tool to check if a certain functionality is present

I happen to agree and would love it to be auto generated based on what is defined in the plugin check. IMHO wrong documentation is a far worse sin than lack of documentation. I do not think the community (in general, there are obviously exceptions) has the appetite to support the effort involved manually keeping this up to date. We have a hard enough time getting people to do things like rebase PRs and proper changelog entries.

@majormoses majormoses requested a review from eheydrick June 24, 2017 16:33
@majormoses
Copy link
Member

@eheydrick thoughts?

@majormoses
Copy link
Member

@GhostLyrics looks like this fell off both our radars I am game if you are to review this and see if we can get some traction here. @mbbroberg what are your thoughts on how to proceed with documentation across all the plugins? I tend to view cli/api as the most important to be right as external documentation often get's out of date so I think we should make effort to ensure that there are useful descriptions on every param and look for a self documentation solution such as yard but at this point I will take pretty much any improvement over the current state.

@majormoses
Copy link
Member

Last ping before closing.

@GhostLyrics
Copy link
Author

I'm not sure what you expect me to do. Just like last time, you asked another member of the Sensu team for input and did not receive an answer. It does not look like you have a specific request for me or hints how you would like this to proceed. 😞

@majormoses
Copy link
Member

majormoses commented Feb 23, 2018

I asked for both of your inputs, apologies if that was not conveyed properly. I was just trying to loop in another opinion but in his absence I think we can proceed. I apologize that no one is responding I don't work for sensu and neither does eric so I tried to loop in @mbbroberg who does. Even in the absence of a wider decision on how we should do things in general I say we move forward with smaller shorter term goals we can get accomplished. Incremental improvement is the name of the game.

@majormoses
Copy link
Member

majormoses commented Feb 23, 2018

Regarding specific hints and feedback I made several comments that you did not respond on so I was waiting on that. I am pinging lots of people through other channels as this really just needs to be discussed, reviewed, and starting to get shit done. I feel your pain and trying to to see if we can get another set of eyes on this.

@mattyjones
Copy link
Member

@GhostLyrics

Again, sorry for the long stall on this, no excuses. I very much appreciate your willingness to help and have been where you sit when I pr to other projects and I understand the frustration. Moving forward the team has more channels and cohesion now so matters like this are quickly getting addressed and communication is more frequent and concerted.

To the meat of the PR, here are my thoughts from experience and personal taste.

I do not think options should be called out in the README, that means we have to maintain docs in 2 specific spots and that is not going to happen, we all know it. I firmly agree with your thoughts on the description being more elaborated though. When it comes to documentation in general what are your thoughts on yard? I personally would like to see, and am going to make a concerted effort myself to try and fully document the plugin. This would supplement the --help flag with valid docs as shown here for example.

Yard will go a long way to being easier to maintain as it is code, right in the plugin, it is/can be automated, and it still only requires touching 1 file (the plugin). This is all outside of the fact that yard/rdoc is the ruby standard for documenting code and if needed can be compiled locally.

@mbbroberg
Copy link

From @GhostLyrics:

simply not great documentation if I either have to dive into the code OR install the tool to check if a certain functionality is present

From @majormoses:

I happen to agree and would love it to be auto generated based on what is defined in the plugin check.

@majormoses you're also a major advocate for iterative improvement. I think even manual repetition of what should be autogenerated can and should make it to the README in the meantime.

My opinion is :shipit: and accept we have room to grow. At least we help people in the meantime.

Follow up I'm reading:

@GhostLyrics
Copy link
Author

@mattyjones I'm not sure what I'm supposed to see at your linked example. From what I see, this is the exact content of the Readme.markdown with slightly different styling from the Github one.

This would supplement the --help flag with valid docs as shown here for example.


means we have to maintain docs in 2 specific spots and that is not going to happen, we all know it.

I don't know. Make a concerted effort to address old issues and refuse to accept PRs with code without docs updates as soon as the repository has been converted to the standardized docs. But I probably imagine this being easier than it is.


Yard will go a long way to being easier to maintain as it is code, right in the plugin, it is/can be automated

With a quick search I've found some examples how this is supposed to work for classes and more OOP related things. I didn't understand how your proposal addresses the myriad of command line parameters and how you'd add example commands. Furthermore, in my very subjective opinion, this is almost the same as looking at the source code directly, but that is debatable.


@majormoses the tabular approach fails pretty obviously for long lines, as in your example…

If we wish to do document all this in the readme I might suggest a more tabular approach like this:


I tend to view cli/api as the most important to be right as external documentation often get's out of date

Again, I'll add: I hate to have to execute (or read) your code to know what it does. If things can be done automatically with yard, I'd enjoy automatically generating it where it's possible (mind you that in some repos there are non-Ruby scripts)

@majormoses
Copy link
Member

majormoses commented Feb 23, 2018

@majormoses you're also a major advocate for iterative improvement.

In general I am except in the case of where making incremental improvement of something does more harm than good because lack of design. In this case I am torn. I feel any manual documentation that does not auto update as something that is doomed to be outdated which is worse than having nothing. I'd prefer if we start with using something like yard but if we can't come to an agreement now I agree that we just accept it and work later on making auto generated from annotated comments as this is more likely to succeed in the long run.

With a quick search I've found some examples how this is supposed to work for classes and more OOP related things. I didn't understand how your proposal addresses the myriad of command line parameters and how you'd add example commands. Furthermore, in my very subjective opinion, this is almost the same as looking at the source code directly, but that is debatable

I think that was a bad example I think looking at the annotated code here which results in something like this is a far better example. This is much more applicable example to what we would need to deal the annotated comments can be found here

The benefit of this approach is that it is annotated right next to the code that it affects it. This benefits users willing to jump into source as well as those who would not want to look at the source. Is this still too unfriendly for a user? I realize I have a very skewed view as I more often jump to the source or --help than documentation for this stuff in general as static documentation is often out of date or lacking.

I don't know. Make a concerted effort to address old issues

I am sorry you feel we do not make an effort to address old issues, I give a lot of time to this community primarily on my personal time as well as what I can justify at work. Unfortunately there is very little support from sensu inc currently with plugins and this is pretty much an entirely led community effort with very few active community maintainers. I really do appreciate the efforts you have put into improving the community and apologize if I sometimes come off as a bit rough around the edges. Some of that I can own as my personality and part of it stems from the lack of active maintenance of over 200 repos that I am pretty much the only active maintainer on a consistent basis.

refuse to accept PRs with code without docs updates as soon as the repository has been converted to the standardized docs. But I probably imagine this being easier than it is.

In an ideal world this would definitely be my stance and I share your thoughts. This is not something we can really start doing overnight, it needs to be thought out, written up, and distributed to all our PR templates with links to explain to users how to document correctly. After all said and done I think this will lead to two scenarios:

  • Additional work on maintainers to document when users do not
  • Much higher close without merge ratio

I have started to take a similar approach to testing and I can assure you this does in fact lead to both of those scenarios above. I am not saying I am unwilling to do the same here but I find it more justifiable to say if you can't take 2 minutes to show redacted input and output telling me that it is working then it's too high of a risk to merge. I am certainly willing to take that stance on docs as well in the future but not right away without getting all that legwork done first.

@majormoses the tabular approach fails pretty obviously for long lines, as in your example…

My example is in a code block to show the formatting how it would look in an editor but when actually in the readme markdown will render it with line wrapping. It's not exactly perfect but I don't think it looks any worse than having it free-form and least provides some structure and potentially parsable data but that might just be me. Here is an example that has long lines that I manage outside of the sensu community. Before it is pointed out that it is indeed manual here maintaining a single repos documentation is much more likely to succeed than 200 and that particular example would be very hard to do that where this would be much simpler.

I'd enjoy automatically generating it where it's possible (mind you that in some repos there are non-Ruby scripts)

Ya I agree we probably need something different for other languages. I think in the context of sensu2 we may start splitting out all the non ruby checks into other repos since the ruby runtime will no longer be required but I agree that we need to come up with a solution for those both in the short term and longer term. Specifically with (sh|bash) I don't know of a good solution for that but I know python and go also have similar projects to yard for auto documentation generation.

@majormoses
Copy link
Member

So another thought that I had was to make a rake task for each plugin script and handler and have it drop the --help useage into a file so you don't have to install it to view it or look at source code would that be any better.

@majormoses
Copy link
Member

Closing due to inactivity

@majormoses majormoses closed this Aug 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants